-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mrdox-test supports yml configs. #112
mrdox-test supports yml configs. #112
Conversation
source/test/Config.cpp
Outdated
// Copyright (c) 2023 Klemens D. Morgenstern | ||
// | ||
// Distributed under the Boost Software License, Version 1.0. (See accompanying | ||
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems I have to repeat myself. We are not using the Boost license, Please follow the example set in the other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, IDO template
source/test/Config.cpp
Outdated
} | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline
source/test/Config.hpp
Outdated
|
||
llvm::Expected<std::vector<TestConfig>> | ||
loadTestConfig(llvm::StringRef dir, | ||
llvm::StringRef file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the established style
source/test/Config.cpp
Outdated
{ | ||
auto fileText = llvm::MemoryBuffer::getFile(filePath); | ||
if(! fileText) | ||
return makeError(fileText.getError().message(), " when loading file '", filePath, "' "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can just put fileText
here instead of fileText.getError().message()
. If not, we can add a nice
overload for it.
source/test/Config.cpp
Outdated
clang::mrdox::TestConfig> | ||
{ | ||
static void mapping(IO &io, | ||
clang::mrdox::TestConfig& f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be best if we did not have two files with the same name anywhere, we already have Config.cpp in api
source/test/Config.cpp
Outdated
if (!fs::exists(filePath)) | ||
{ | ||
filePath = dir; | ||
filePath += "/mrdox-test.yml"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have to use path::append
not +=
cxxstd: c++20 | ||
... | ||
--- | ||
cxxstd: c++17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... no, I don't think we should invent a new yml file format that has multiple yml configurations inside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a new yaml format, that's standard yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I see that but still.... where are you
source/test/SingleFile.hpp
Outdated
{ | ||
char buf[64]; | ||
snprintf(buf, 63, "-std=%s", tc.cxxstd.c_str()); | ||
cmds.emplace_back(buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use the existing style
source/test/Config.cpp
Outdated
return makeError(fileText.getError().message(), " when loading file '", filePath, "' "); | ||
llvm::yaml::Input yin(**fileText); | ||
|
||
std::vector<TestConfig> res; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should only be one config in the yml file. That is, given the files "1.cpp" and "1.xml", if there is a file named "1.yml" then it should contain a single config and be loaded and applied to the test instead of the defaulted config. If "1.yml" does not exist, then if there is a filed called "config.yml" located in the same directory then that should be loaded and applied to the test (you can load it once when iterating the directory and re-use it for all tests in that dir and its children).
Otherwise, if there is no 1.yml, and no config.yml in any of its parent directories, then it should do what it does now with a default config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter part does happen. Allowing multiple configs per yml doesn't cost us much, but allows us to quickly add more tests. This is particularly helpful is a certain functionality doesn't work with a given flag OR on a standard.
An issue reporter can then just add his case for an existing test.
bb83035
to
08fd7fb
Compare
source/test/SingleFile.hpp
Outdated
@@ -15,6 +15,7 @@ | |||
#include <string> | |||
#include <utility> | |||
#include <vector> | |||
#include "TestConfig.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
includes should be ordered most specific -> most general
source/test/TestConfig.cpp
Outdated
|
||
template<> | ||
struct llvm::yaml::MappingTraits< | ||
clang::mrdox::TestConfig> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait a minute I think there's some confusion here, the tests are supposed to use the same YML configuration as the tool. Not a whole new YML configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a different config object.
source/test/SingleFile.hpp
Outdated
cmds.emplace_back("-std=c++20"); | ||
{ | ||
char buf[64]; | ||
snprintf(buf, 63, "-std=%s", tc.cxxstd.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this will create warnings on msvc.. prefer using llvm::raw_string_ostream
08fd7fb
to
812e33c
Compare
@vinnie rebased. |
4e9e12a
to
8571b1c
Compare
8571b1c
to
9b398e5
Compare
9b398e5
to
95fc313
Compare
Already supported now |
This supports multiple documents per config (because yaml) so one config can create multiple runs with a single yaml file.